Draft: Download manager #7
Conversation
cyberphantom52
requested review from
jntesteves,
mirkobrombin,
edfloreshz and
TheEvilSkeleton
cyberphantom52
force-pushed
the
download_manager
branch
from
1dce7d0 to
748106a
Compare
cyberphantom52
force-pushed
the
download_manager
branch
from
92cb6c6 to
9c29890
Compare
This is to make sure we don't block the runtime.
This is the idomatic way
| #[error("Oneshot: {0}")] | ||
| Oneshot(#[from] tokio::sync::oneshot::error::RecvError), |
There was a problem hiding this comment.
I don't like having to add this specific error type.
There was a problem hiding this comment.
Maybe, not sure, RecvError could map to a more general channel cancellation variant instead of adding a dedicated Oneshot error
- added missing impl for GPTK - fixed warnings and clippy
cyberphantom52
force-pushed
the
download_manager
branch
from
cb9d812 to
3fe1e92
Compare
This fixes a bug where if we didn't store the handle to the download somewhere it would automatically cancel the download
This will help with swapping HTTP backends in the future
cyberphantom52
force-pushed
the
download_manager
branch
from
da3d4b1 to
8354cc9
Compare
| let mut response = client | ||
| .get(req.url().as_ref()) | ||
| .send() | ||
| .await? | ||
| .error_for_status()?; |
There was a problem hiding this comment.
We might want to use config.user_agent() to set the request’s User‑Agent header, here
| status: status_tx, | ||
| cancel: cancel.clone(), | ||
| config, | ||
| last_progress_update: Instant::now(), |
There was a problem hiding this comment.
Might be a good idea to initialize last_progress_update so the first InProgress status is sent immediately?
| for attempt in 0..=(max_attempts + 1) { | ||
| if attempt > max_attempts { | ||
| req.fail(Error::Download(DownloadError::RetriesExhausted { | ||
| last_error_msg: last_error | ||
| .as_ref() | ||
| .map(ToString::to_string) | ||
| .unwrap_or_else(|| "Unknown Error".to_string()), | ||
| })); | ||
| return; |
There was a problem hiding this comment.
Maybe consider iterating 0..=max_attempts and handling the exhausted case when attempt == max_attempts
| // Basic exponential backoff | ||
| let delay_ms = 1000 * 2u64.pow(attempt as u32 - 1); | ||
| let delay = Duration::from_millis(delay_ms); |
There was a problem hiding this comment.
Maybe cap the exponential backoff and sprinkle in some jitter so retries don’t wait forever.
| for attempt in 0..=(max_attempts + 1) { | ||
| if attempt > max_attempts { | ||
| req.fail(Error::Download(DownloadError::RetriesExhausted { | ||
| last_error_msg: last_error | ||
| .as_ref() | ||
| .map(ToString::to_string) | ||
| .unwrap_or_else(|| "Unknown Error".to_string()), | ||
| })); | ||
| return; | ||
| } | ||
|
|
||
| if attempt > 0 { | ||
| req.retry(); | ||
| // Basic exponential backoff | ||
| let delay_ms = 1000 * 2u64.pow(attempt as u32 - 1); | ||
| let delay = Duration::from_millis(delay_ms); | ||
|
|
||
| tokio::select! { | ||
| _ = tokio::time::sleep(delay) => {}, | ||
| _ = req.cancel.cancelled() => { | ||
| req.cancel(); | ||
| return; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| match download(client.clone(), &mut req).await { |
There was a problem hiding this comment.
A quick check if the request is canceled at the start of each attempt might let us exit before starting the network request
| drop(file); // Manually drop the file handle to ensure that deletion doesn't fail | ||
| tokio::fs::remove_file(&req.destination()).await?; | ||
| return Err(Error::Download(DownloadError::Cancelled)); |
There was a problem hiding this comment.
Perhaps ignore any errors from remove_file so cancellation doesn’t hide the real problem
Same at L106‑108
| pub fn active_downloads(&self) -> usize { | ||
| // -1 because the dispatcher thread is always running | ||
| self.tracker.len() - 1 |
There was a problem hiding this comment.
Using saturating_sub(1) could keep active_downloads from goinig negative if the dispatcher isn't running
Implements #6
TODO: